You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This lets you easily create expressions with a specific structure.
For example, if you want to have $$f(x_1, x_2) + exp(- g(x_3) \cdot g(x_3))$$, this PR will let you.
There are some remaining issues though that we need to fix. It seems there is an undefined reference error when simplifying that I think is likely due to get_tree stitching the inner expressions together, whereby some function (that I haven't figured out yet) mutates the resultant expressions.
This should never happen, and is always avoided by mutate! because of the fact that they call the new function get_contents_for_mutation which prevents this. But I guess there are some other functions which also need this guard in place.
Fixes aliasing issues
This was a very bad bug that I just happened to identify as part of this PR ([1],[2]). Basically, the crossover operation would sometimes happen between a population member and itself. There was no check to prevent the crossover from sampling the exact same member from the population!
In addition to weird effects from actually performing a crossover operation on the same expression, this also meant that multiple individuals in a population could end up being the same. This means that all downstream operations (including ones ran in parallel!) would be susceptible to aliasing issues.
In short, this was a nasty bug, and it is surprising that it only showed up explicitly when implementing the BlueprintExpression. I wonder if this was the source of various crashes (especially for long-running jobs) over the past 1+ years, and I was just never able to identify the source of the race condition.
Anyways, this seems like it is fixed now, because we copy population members immediately after sampling.
Fix maxsize issues
There were some issues hanging around that had to do with maxsize being violated. It turns out this was simply an issue of a < appearing where a <= should have been. This has been fixed.
TODO:
Fix aliasing issue due to some function mutating the combined expression rather than working only on a single inner expression at a time.
One idea is that I could create a new node type, ReadOnlyNode, which throws an error for any setproperty!. Calling get_tree on ConstrainedExpression will return this node type.
Fix a name for this. Currently it is ConstrainedExpression but I'm not a fan of this. ConstrainedStructuredExpression is too long. Maybe MultiExpression?
Document the new type.
Add unittests. Want to reach 100% diff coverage as there's a lot of new features here.
I wonder if the aliasing could be due to the use of copy_node rather than copy in the codebase? Note that copy calls copy_node but not the other way around. Nope, the expression version of copy_node simply calls copy.
Very odd. It's extremely worrying if there's an aliasing issue though. I can't immediately see where it might be coming from.
It looks like sometimes, crossover attempts to cross the same expression with itself. Perhaps that is why this issue arises? Not because of the fact that there is aliasing, but because copy corrects any undefined fields in the tree?
Yep, it looks like crossover applied to the exact same population member is enough to cause aliasing! Because it will put both "children" back into the population. If that's the same member, then that will mean that multiple population members are the same!
MilesCranmer
changed the title
Add structured expressions
Create BlueprintExpression for providing a pre-defined functional structure and constraints
Oct 14, 2024
A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).
MilesCranmer
changed the title
Create BlueprintExpression for providing a pre-defined functional structure and constraints
Create TemplateExpression for providing a pre-defined functional structure and constraints
Oct 15, 2024
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MilesCranmer
changed the title
TemplateExpressionThis lets you easily create expressions with a specific structure.
For example, if you want to have$$f(x_1, x_2) + exp(- g(x_3) \cdot g(x_3))$$ , this PR will let you.
There are some remaining issues though that we need to fix. It seems there is an undefined reference error when simplifying that I think is likely due toget_treestitching the inner expressions together, whereby some function (that I haven't figured out yet) mutates the resultant expressions.This should never happen, and is always avoided bymutate!because of the fact that they call the new functionget_contents_for_mutationwhich prevents this. But I guess there are some other functions which also need this guard in place.Fixes aliasing issues
This was a very bad bug that I just happened to identify as part of this PR ([1],[2]). Basically, the crossover operation would sometimes happen between a population member and itself. There was no check to prevent the crossover from sampling the exact same member from the population!
In addition to weird effects from actually performing a crossover operation on the same expression, this also meant that multiple individuals in a population could end up being the same. This means that all downstream operations (including ones ran in parallel!) would be susceptible to aliasing issues.
In short, this was a nasty bug, and it is surprising that it only showed up explicitly when implementing the BlueprintExpression. I wonder if this was the source of various crashes (especially for long-running jobs) over the past 1+ years, and I was just never able to identify the source of the race condition.
Anyways, this seems like it is fixed now, because we copy population members immediately after sampling.
Fix
maxsizeissuesThere were some issues hanging around that had to do with
maxsizebeing violated. It turns out this was simply an issue of a<appearing where a<=should have been. This has been fixed.TODO:
ReadOnlyNode, which throws an error for anysetproperty!. Callingget_treeonConstrainedExpressionwill return this node type.ReadOnlyNodeforStructuredExpressionget_treeaccess SymbolicML/DynamicExpressions.jl#105ConstrainedExpressionbut I'm not a fan of this.ConstrainedStructuredExpressionis too long. MaybeMultiExpression?